Skip to content

mips: zero-extend microMIPS LWM16/SWM16 reglist offset#2991

Draft
Samin061 wants to merge 1 commit into
capstone-engine:nextfrom
Samin061:mips-mm-reglist-neg-shift
Draft

mips: zero-extend microMIPS LWM16/SWM16 reglist offset#2991
Samin061 wants to merge 1 commit into
capstone-engine:nextfrom
Samin061:mips-mm-reglist-neg-shift

Conversation

@Samin061

@Samin061 Samin061 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

Reopening #2989 with the fix reworked per review. That branch was force-pushed so GitHub won't let it be reopened, hence a fresh PR.

DecodeMemMMReglistImm4Lsl2 decodes the offset of the microMIPS LWM16/SWM16 instructions. The ISA defines the address as:

vAddr = zero_extend(offset||0^2) + GPR[sp]

so the 4-bit offset field is concatenated with two zero bits and zero-extended. The MMR6 path already reads it unsigned via fieldFromInstruction_4, but the non-MMR6 path used SignExtend32((Insn & 0xf), 4). Whenever the field's top bit is set (field >= 8) that makes Offset negative, so Offset << 2 is a left shift of a negative value (UB), and the decoded offset came out negative instead of the zero-extended value the ISA requires.

The fix reads the field with fieldFromInstruction_4(Insn, 0, 4), matching the MMR6 branch and the ISA. The shift is then always applied to a non-negative value.

Test plan

Built with -fsanitize=undefined -fno-sanitize-recover=undefined and confirmed both directions:

  • before: cstool micromips 4508 and test_poc abort at arch/Mips/MipsDisassembler.c:2432 with runtime error: left shift of negative value -8.
  • after: test_poc exits cleanly and cstool micromips 4508 prints lwm16 $s0, $ra, 0x20($sp) (8 << 2 == 0x20, the zero-extended value).

Added test_ub_shift_mips_mm_reglist to tests/integration/test_poc.c, next to the existing test_ub_shift_sh_dsp_p signed-shift regression.

Closing issues

@Samin061 Samin061 mentioned this pull request Jul 9, 2026
Closed
2 tasks
@Samin061

Samin061 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@Rot127 this is the reworked version of #2989. Two things from your review:

  • Switched the non-MMR6 path to fieldFromInstruction_4(Insn, 0, 4) instead of a raw mask, so it reads the field unsigned the same way the MMR6 branch and the ISA (zero_extend(offset||0^2)) do.
  • Validated both directions under -fsanitize=undefined: on the old code cstool micromips 4508 and test_poc abort at MipsDisassembler.c:2432 with 'left shift of negative value -8'; with the fix test_poc exits clean and 4508 decodes to lwm16 $s0, $ra, 0x20($sp) (8 << 2 == 0x20).

I do lean on a tool for research here and there, but I ran the before/after myself to confirm this one holds up.

@github-actions github-actions Bot added the Mips Arch label Jul 9, 2026
@Samin061

Samin061 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Gentle ping on this. To answer the ISA-ordering point directly: the field is zero-extended and then shifted (vAddr = zero_extend(offset||0^2)), not sign-extended, so fieldFromInstruction_4(Insn, 0, 4) is the right read and it lines up with the MMR6 branch above it.

Re-ran the before/after under -fsanitize=undefined to double-check it holds: on the old SignExtend32 path micromips 4508 aborts at MipsDisassembler.c:2432 with left shift of negative value -8; with the fix test_poc exits clean and 4508 decodes to lwm16 $s0, $ra, 0x20($sp) (8 << 2 == 0x20). Happy to adjust if you'd rather shape it differently.

Rot127
Rot127 requested changes Jul 13, 2026
View reviewed changes

@Rot127 Rot127 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. I am a little more busy than usual these days.

Please add the instruction to the tests in tests/issues/issues.yaml as well.

There are also failing tests and it needs a rebase.

@Samin061
mips: zero-extend microMIPS LWM16/SWM16 reglist offset
13ca6f3 
The ISA defines the offset as zero_extend(offset||0^2), but the non-MMR6
path in DecodeMemMMReglistImm4Lsl2 sign-extended the 4-bit field before
the << 2. When the field's top bit is set that makes Offset negative, so
Offset << 2 is a left shift of a negative value (UB), and the decoded
offset came out negative instead of the zero-extended value the ISA
requires.

Use fieldFromInstruction_4(Insn, 0, 4) to extract the field unsigned,
matching the MMR6 branch and the ISA. The shift is now always applied to
a non-negative value.

Verified under -fsanitize=undefined: before, cstool micromips 4508 and
test_poc abort at MipsDisassembler.c:2432 with 'left shift of negative
value -8'; after, test_poc exits clean and cstool prints
'lwm16 $s0, $ra, 0x20($sp)' (8 << 2 == 0x20).

Signed-off-by: bibi samina <sam@bugqore.com>
@Samin061 Samin061 force-pushed the mips-mm-reglist-neg-shift branch from 984fec0 to 13ca6f3 Compare July 13, 2026 13:06
@Samin061

Samin061 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto next and added the LWM16 case to tests/issues/issues.yaml (4508 -> lwm16 $s0, $ra, 0x20($sp)). The failing tests were the conflict against the xtensa test that landed on next in the meantime; resolved that in the rebase so test_poc keeps both cases. Should be green now.

@Rot127 Rot127 marked this pull request as draft July 13, 2026 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Rot127 Rot127 Rot127 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

Mips Arch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Samin061 @Rot127